Skip to content

Conversation

kmpeng
Copy link
Contributor

@kmpeng kmpeng commented May 14, 2025

Tasks completed:

  • Pattern matchselect(fcmp(dot(p2, p3), 0), p1, -p1) to faceforward(p1, p2, p3)
  • Add pattern matching tests to prelegalizercombiner-select-to-faceforward.mir and faceforward.ll
  • Add CL extension error test llvm/test/CodeGen/SPIRV/opencl/faceforward-error.ll
  • Add CL extension test for no pattern matching in llvm/test/CodeGen/SPIRV/opencl/faceforward.ll

Closes #137255.

@llvmbot llvmbot added clang Clang issues not falling into any other category backend:X86 clang:headers Headers provided by Clang, e.g. for intrinsics llvm:globalisel HLSL HLSL Language Support backend:SPIR-V labels May 14, 2025
@llvmbot
Copy link
Member

llvmbot commented May 14, 2025

@llvm/pr-subscribers-llvm-globalisel
@llvm/pr-subscribers-backend-x86

@llvm/pr-subscribers-backend-spir-v

Author: Kaitlin Peng (kmpeng)

Changes

Tasks completed:

  • instCombine select(fcmp(dot(p2, p3), 0), p1, 0 - p1) to faceforward(p1, p2, p3)
  • Remove HLSL faceforward intrinsic's SPIR-V fast path
  • Add instCombine tests to prelegalizercombiner-select-to-faceforward.mir and faceforward.ll
  • Add CL extension error test llvm/test/CodeGen/SPIRV/opencl/faceforward-error.ll

Closes #137255.


Patch is 25.07 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/139959.diff

7 Files Affected:

  • (modified) clang/lib/Headers/hlsl/hlsl_intrinsic_helpers.h (-4)
  • (modified) clang/test/CodeGenHLSL/builtins/faceforward.hlsl (+40-16)
  • (modified) llvm/lib/Target/SPIRV/SPIRVCombine.td (+8-1)
  • (modified) llvm/lib/Target/SPIRV/SPIRVPreLegalizerCombiner.cpp (+100-11)
  • (added) llvm/test/CodeGen/SPIRV/GlobalISel/InstCombine/prelegalizercombiner-select-to-faceforward.mir (+63)
  • (modified) llvm/test/CodeGen/SPIRV/hlsl-intrinsics/faceforward.ll (+42-8)
  • (added) llvm/test/CodeGen/SPIRV/opencl/faceforward-error.ll (+13)
diff --git a/clang/lib/Headers/hlsl/hlsl_intrinsic_helpers.h b/clang/lib/Headers/hlsl/hlsl_intrinsic_helpers.h
index 4eb7b8f45c85a..e1996fd3e30ed 100644
--- a/clang/lib/Headers/hlsl/hlsl_intrinsic_helpers.h
+++ b/clang/lib/Headers/hlsl/hlsl_intrinsic_helpers.h
@@ -127,11 +127,7 @@ template <typename T> constexpr vector<T, 4> lit_impl(T NDotL, T NDotH, T M) {
 }
 
 template <typename T> constexpr T faceforward_impl(T N, T I, T Ng) {
-#if (__has_builtin(__builtin_spirv_faceforward))
-  return __builtin_spirv_faceforward(N, I, Ng);
-#else
   return select(dot(I, Ng) < 0, N, -N);
-#endif
 }
 
 template <typename T> constexpr T ldexp_impl(T X, T Exp) {
diff --git a/clang/test/CodeGenHLSL/builtins/faceforward.hlsl b/clang/test/CodeGenHLSL/builtins/faceforward.hlsl
index d2ece57aba4ae..f12f75f384964 100644
--- a/clang/test/CodeGenHLSL/builtins/faceforward.hlsl
+++ b/clang/test/CodeGenHLSL/builtins/faceforward.hlsl
@@ -12,8 +12,11 @@
 // CHECK: %hlsl.select.i = select reassoc nnan ninf nsz arcp afn i1 %cmp.i, half %{{.*}}, half %fneg.i
 // CHECK: ret half %hlsl.select.i
 // SPVCHECK-LABEL: test_faceforward_half
-// SPVCHECK: %spv.faceforward.i = call reassoc nnan ninf nsz arcp afn noundef half @llvm.spv.faceforward.f16(half %{{.*}}, half %{{.*}}, half %{{.*}})
-// SPVCHECK: ret half %spv.faceforward.i
+// SPVCHECK: %hlsl.dot.i = fmul reassoc nnan ninf nsz arcp afn half %{{.*}}, %{{.*}}
+// SPVCHECK: %cmp.i = fcmp reassoc nnan ninf nsz arcp afn olt half %hlsl.dot.i, 0xH0000
+// SPVCHECK: %fneg.i = fneg reassoc nnan ninf nsz arcp afn half %{{.*}}
+// SPVCHECK: %hlsl.select.i = select reassoc nnan ninf nsz arcp afn i1 %cmp.i, half %{{.*}}, half %fneg.i
+// SPVCHECK: ret half %hlsl.select.i
 half test_faceforward_half(half N, half I, half Ng) { return faceforward(N, I, Ng); }
 
 // CHECK-LABEL: test_faceforward_half2
@@ -23,8 +26,11 @@ half test_faceforward_half(half N, half I, half Ng) { return faceforward(N, I, N
 // CHECK: %hlsl.select.i = select reassoc nnan ninf nsz arcp afn i1 %cmp.i, <2 x half> %{{.*}}, <2 x half> %fneg.i
 // CHECK: ret <2 x half> %hlsl.select.i
 // SPVCHECK-LABEL: test_faceforward_half2
-// SPVCHECK: %spv.faceforward.i = call reassoc nnan ninf nsz arcp afn noundef <2 x half> @llvm.spv.faceforward.v2f16(<2 x half> %{{.*}}, <2 x half> %{{.*}}, <2 x half> %{{.*}})
-// SPVCHECK: ret <2 x half> %spv.faceforward.i
+// SPVCHECK: %hlsl.dot.i = call reassoc nnan ninf nsz arcp afn half @llvm.spv.fdot.v2f16(<2 x half> %{{.*}}, <2 x half> %{{.*}})
+// SPVCHECK: %cmp.i = fcmp reassoc nnan ninf nsz arcp afn olt half %hlsl.dot.i, 0xH0000
+// SPVCHECK: %fneg.i = fneg reassoc nnan ninf nsz arcp afn <2 x half> %{{.*}}
+// SPVCHECK: %hlsl.select.i = select reassoc nnan ninf nsz arcp afn i1 %cmp.i, <2 x half> %{{.*}}, <2 x half> %fneg.i
+// SPVCHECK: ret <2 x half> %hlsl.select.i
 half2 test_faceforward_half2(half2 N, half2 I, half2 Ng) { return faceforward(N, I, Ng); }
 
 // CHECK-LABEL: test_faceforward_half3
@@ -34,8 +40,11 @@ half2 test_faceforward_half2(half2 N, half2 I, half2 Ng) { return faceforward(N,
 // CHECK: %hlsl.select.i = select reassoc nnan ninf nsz arcp afn i1 %cmp.i, <3 x half> %{{.*}}, <3 x half> %fneg.i
 // CHECK: ret <3 x half> %hlsl.select.i
 // SPVCHECK-LABEL: test_faceforward_half3
-// SPVCHECK: %spv.faceforward.i = call reassoc nnan ninf nsz arcp afn noundef <3 x half> @llvm.spv.faceforward.v3f16(<3 x half> %{{.*}}, <3 x half> %{{.*}}, <3 x half> %{{.*}})
-// SPVCHECK: ret <3 x half> %spv.faceforward.i
+// SPVCHECK: %hlsl.dot.i = call reassoc nnan ninf nsz arcp afn half @llvm.spv.fdot.v3f16(<3 x half> %{{.*}}, <3 x half> %{{.*}})
+// SPVCHECK: %cmp.i = fcmp reassoc nnan ninf nsz arcp afn olt half %hlsl.dot.i, 0xH0000
+// SPVCHECK: %fneg.i = fneg reassoc nnan ninf nsz arcp afn <3 x half> %{{.*}}
+// SPVCHECK: %hlsl.select.i = select reassoc nnan ninf nsz arcp afn i1 %cmp.i, <3 x half> %{{.*}}, <3 x half> %fneg.i
+// SPVCHECK: ret <3 x half> %hlsl.select.i
 half3 test_faceforward_half3(half3 N, half3 I, half3 Ng) { return faceforward(N, I, Ng); }
 
 // CHECK-LABEL: test_faceforward_half4
@@ -45,8 +54,11 @@ half3 test_faceforward_half3(half3 N, half3 I, half3 Ng) { return faceforward(N,
 // CHECK: %hlsl.select.i = select reassoc nnan ninf nsz arcp afn i1 %cmp.i, <4 x half> %{{.*}}, <4 x half> %fneg.i
 // CHECK: ret <4 x half> %hlsl.select.i
 // SPVCHECK-LABEL: test_faceforward_half4
-// SPVCHECK: %spv.faceforward.i = call reassoc nnan ninf nsz arcp afn noundef <4 x half> @llvm.spv.faceforward.v4f16(<4 x half> %{{.*}}, <4 x half> %{{.*}}, <4 x half> %{{.*}})
-// SPVCHECK: ret <4 x half> %spv.faceforward.i
+// SPVCHECK: %hlsl.dot.i = call reassoc nnan ninf nsz arcp afn half @llvm.spv.fdot.v4f16(<4 x half> %{{.*}}, <4 x half> %{{.*}})
+// SPVCHECK: %cmp.i = fcmp reassoc nnan ninf nsz arcp afn olt half %hlsl.dot.i, 0xH0000
+// SPVCHECK: %fneg.i = fneg reassoc nnan ninf nsz arcp afn <4 x half> %{{.*}}
+// SPVCHECK: %hlsl.select.i = select reassoc nnan ninf nsz arcp afn i1 %cmp.i, <4 x half> %{{.*}}, <4 x half> %fneg.i
+// SPVCHECK: ret <4 x half> %hlsl.select.i
 half4 test_faceforward_half4(half4 N, half4 I, half4 Ng) { return faceforward(N, I, Ng); }
 
 // CHECK-LABEL: test_faceforward_float
@@ -56,8 +68,11 @@ half4 test_faceforward_half4(half4 N, half4 I, half4 Ng) { return faceforward(N,
 // CHECK: %hlsl.select.i = select reassoc nnan ninf nsz arcp afn i1 %cmp.i, float %{{.*}}, float %fneg.i
 // CHECK: ret float %hlsl.select.i
 // SPVCHECK-LABEL: test_faceforward_float
-// SPVCHECK: %spv.faceforward.i = call reassoc nnan ninf nsz arcp afn noundef float @llvm.spv.faceforward.f32(float %{{.*}}, float %{{.*}}, float %{{.*}})
-// SPVCHECK: ret float %spv.faceforward.i
+// SPVCHECK: %hlsl.dot.i = fmul reassoc nnan ninf nsz arcp afn float %{{.*}}, %{{.*}}
+// SPVCHECK: %cmp.i = fcmp reassoc nnan ninf nsz arcp afn olt float %hlsl.dot.i, 0.000000e+00
+// SPVCHECK: %fneg.i = fneg reassoc nnan ninf nsz arcp afn float %{{.*}}
+// SPVCHECK: %hlsl.select.i = select reassoc nnan ninf nsz arcp afn i1 %cmp.i, float %{{.*}}, float %fneg.i
+// SPVCHECK: ret float %hlsl.select.i
 float test_faceforward_float(float N, float I, float Ng) { return faceforward(N, I, Ng); }
 
 // CHECK-LABEL: test_faceforward_float2
@@ -67,8 +82,11 @@ float test_faceforward_float(float N, float I, float Ng) { return faceforward(N,
 // CHECK: %hlsl.select.i = select reassoc nnan ninf nsz arcp afn i1 %cmp.i, <2 x float> %{{.*}}, <2 x float> %fneg.i
 // CHECK: ret <2 x float> %hlsl.select.i
 // SPVCHECK-LABEL: test_faceforward_float2
-// SPVCHECK: %spv.faceforward.i = call reassoc nnan ninf nsz arcp afn noundef <2 x float> @llvm.spv.faceforward.v2f32(<2 x float> %{{.*}}, <2 x float> %{{.*}}, <2 x float> %{{.*}})
-// SPVCHECK: ret <2 x float> %spv.faceforward.i
+// SPVCHECK: %hlsl.dot.i = call reassoc nnan ninf nsz arcp afn float @llvm.spv.fdot.v2f32(<2 x float> %{{.*}}, <2 x float> %{{.*}})
+// SPVCHECK: %cmp.i = fcmp reassoc nnan ninf nsz arcp afn olt float %hlsl.dot.i, 0.000000e+00
+// SPVCHECK: %fneg.i = fneg reassoc nnan ninf nsz arcp afn <2 x float> %{{.*}}
+// SPVCHECK: %hlsl.select.i = select reassoc nnan ninf nsz arcp afn i1 %cmp.i, <2 x float> %{{.*}}, <2 x float> %fneg.i
+// SPVCHECK: ret <2 x float> %hlsl.select.i
 float2 test_faceforward_float2(float2 N, float2 I, float2 Ng) { return faceforward(N, I, Ng); }
 
 // CHECK-LABEL: test_faceforward_float3
@@ -78,8 +96,11 @@ float2 test_faceforward_float2(float2 N, float2 I, float2 Ng) { return faceforwa
 // CHECK: %hlsl.select.i = select reassoc nnan ninf nsz arcp afn i1 %cmp.i, <3 x float> %{{.*}}, <3 x float> %fneg.i
 // CHECK: ret <3 x float> %hlsl.select.i
 // SPVCHECK-LABEL: test_faceforward_float3
-// SPVCHECK: %spv.faceforward.i = call reassoc nnan ninf nsz arcp afn noundef <3 x float> @llvm.spv.faceforward.v3f32(<3 x float> %{{.*}}, <3 x float> %{{.*}}, <3 x float> %{{.*}})
-// SPVCHECK: ret <3 x float> %spv.faceforward.i
+// SPVCHECK: %hlsl.dot.i = call reassoc nnan ninf nsz arcp afn float @llvm.spv.fdot.v3f32(<3 x float> %{{.*}}, <3 x float> %{{.*}})
+// SPVCHECK: %cmp.i = fcmp reassoc nnan ninf nsz arcp afn olt float %hlsl.dot.i, 0.000000e+00
+// SPVCHECK: %fneg.i = fneg reassoc nnan ninf nsz arcp afn <3 x float> %{{.*}}
+// SPVCHECK: %hlsl.select.i = select reassoc nnan ninf nsz arcp afn i1 %cmp.i, <3 x float> %{{.*}}, <3 x float> %fneg.i
+// SPVCHECK: ret <3 x float> %hlsl.select.i
 float3 test_faceforward_float3(float3 N, float3 I, float3 Ng) { return faceforward(N, I, Ng); }
 
 // CHECK-LABEL: test_faceforward_float4
@@ -89,6 +110,9 @@ float3 test_faceforward_float3(float3 N, float3 I, float3 Ng) { return faceforwa
 // CHECK: %hlsl.select.i = select reassoc nnan ninf nsz arcp afn i1 %cmp.i, <4 x float> %{{.*}}, <4 x float> %fneg.i
 // CHECK: ret <4 x float> %hlsl.select.i
 // SPVCHECK-LABEL: test_faceforward_float4
-// SPVCHECK: %spv.faceforward.i = call reassoc nnan ninf nsz arcp afn noundef <4 x float> @llvm.spv.faceforward.v4f32(<4 x float> %{{.*}}, <4 x float> %{{.*}}, <4 x float> %{{.*}})
-// SPVCHECK: ret <4 x float> %spv.faceforward.i
+// SPVCHECK: %hlsl.dot.i = call reassoc nnan ninf nsz arcp afn float @llvm.spv.fdot.v4f32(<4 x float> %{{.*}}, <4 x float> %{{.*}})
+// SPVCHECK: %cmp.i = fcmp reassoc nnan ninf nsz arcp afn olt float %hlsl.dot.i, 0.000000e+00
+// SPVCHECK: %fneg.i = fneg reassoc nnan ninf nsz arcp afn <4 x float> %{{.*}}
+// SPVCHECK: %hlsl.select.i = select reassoc nnan ninf nsz arcp afn i1 %cmp.i, <4 x float> %{{.*}}, <4 x float> %fneg.i
+// SPVCHECK: ret <4 x float> %hlsl.select.i
 float4 test_faceforward_float4(float4 N, float4 I, float4 Ng) { return faceforward(N, I, Ng); }
diff --git a/llvm/lib/Target/SPIRV/SPIRVCombine.td b/llvm/lib/Target/SPIRV/SPIRVCombine.td
index 6f726e024de52..78a57146e61d2 100644
--- a/llvm/lib/Target/SPIRV/SPIRVCombine.td
+++ b/llvm/lib/Target/SPIRV/SPIRVCombine.td
@@ -15,8 +15,15 @@ def vector_length_sub_to_distance_lowering : GICombineRule <
   (apply [{ applySPIRVDistance(*${root}, MRI, B); }])
 >;
 
+def vector_select_to_faceforward_lowering : GICombineRule <
+  (defs root:$root),
+  (match (wip_match_opcode G_SELECT):$root,
+          [{ return matchSelectToFaceForward(*${root}, MRI); }]),
+  (apply [{ applySPIRVFaceForward(*${root}, MRI, B); }])
+>;
+
 def SPIRVPreLegalizerCombiner
     : GICombiner<"SPIRVPreLegalizerCombinerImpl",
-                       [vector_length_sub_to_distance_lowering]> {
+                       [vector_length_sub_to_distance_lowering, vector_select_to_faceforward_lowering]> {
     let CombineAllMethodName = "tryCombineAllImpl";
 }
diff --git a/llvm/lib/Target/SPIRV/SPIRVPreLegalizerCombiner.cpp b/llvm/lib/Target/SPIRV/SPIRVPreLegalizerCombiner.cpp
index c96ee6b02491a..82c5cbf75b849 100644
--- a/llvm/lib/Target/SPIRV/SPIRVPreLegalizerCombiner.cpp
+++ b/llvm/lib/Target/SPIRV/SPIRVPreLegalizerCombiner.cpp
@@ -50,6 +50,18 @@ namespace {
 #include "SPIRVGenPreLegalizeGICombiner.inc"
 #undef GET_GICOMBINER_TYPES
 
+static void removeAllUses(Register Reg, MachineRegisterInfo &MRI,
+                          SPIRVGlobalRegistry *GR) {
+  SmallVector<MachineInstr *, 4> UsesToErase(
+      llvm::make_pointer_range(MRI.use_instructions(Reg)));
+
+  // calling eraseFromParent too early invalidates the iterator.
+  for (auto *MIToErase : UsesToErase) {
+    GR->invalidateMachineInstr(MIToErase);
+    MIToErase->eraseFromParent();
+  }
+}
+
 /// This match is part of a combine that
 /// rewrites length(X - Y) to distance(X, Y)
 ///   (f32 (g_intrinsic length
@@ -98,21 +110,98 @@ void applySPIRVDistance(MachineInstr &MI, MachineRegisterInfo &MRI,
 
   SPIRVGlobalRegistry *GR =
       MI.getMF()->getSubtarget<SPIRVSubtarget>().getSPIRVGlobalRegistry();
-  auto RemoveAllUses = [&](Register Reg) {
-    SmallVector<MachineInstr *, 4> UsesToErase(
-        llvm::make_pointer_range(MRI.use_instructions(Reg)));
-
-    // calling eraseFromParent to early invalidates the iterator.
-    for (auto *MIToErase : UsesToErase) {
-      GR->invalidateMachineInstr(MIToErase);
-      MIToErase->eraseFromParent();
-    }
-  };
-  RemoveAllUses(SubDestReg);   // remove all uses of FSUB Result
+  removeAllUses(SubDestReg, MRI, GR); // remove all uses of FSUB Result
   GR->invalidateMachineInstr(SubInstr);
   SubInstr->eraseFromParent(); // remove FSUB instruction
 }
 
+/// This match is part of a combine that
+/// rewrites select(fcmp(dot(I, Ng), 0), N, 0 - N) to faceforward(N, I, Ng)
+///   (vXf32 (g_select
+///             (g_fcmp
+///                (g_intrinsic dot(vXf32 I) (vXf32 Ng)
+///                 0)
+///             (vXf32 N)
+///             (vXf32 g_fsub (0) (vXf32 N))))
+/// ->
+///   (vXf32 (g_intrinsic faceforward
+///             (vXf32 N) (vXf32 I) (vXf32 Ng)))
+///
+bool matchSelectToFaceForward(MachineInstr &MI, MachineRegisterInfo &MRI) {
+  if (MI.getOpcode() != TargetOpcode::G_SELECT)
+    return false;
+
+  // Check if select's condition is a comparison between a dot product and 0.
+  Register CondReg = MI.getOperand(1).getReg();
+  MachineInstr *CondInstr = MRI.getVRegDef(CondReg);
+  if (!CondInstr || CondInstr->getOpcode() != TargetOpcode::G_FCMP)
+    return false;
+
+  Register DotReg = CondInstr->getOperand(2).getReg();
+  MachineInstr *DotInstr = MRI.getVRegDef(DotReg);
+  if (DotInstr->getOpcode() != TargetOpcode::G_FMUL &&
+      (DotInstr->getOpcode() != TargetOpcode::G_INTRINSIC ||
+       cast<GIntrinsic>(DotInstr)->getIntrinsicID() != Intrinsic::spv_fdot))
+    return false;
+
+  Register CondZeroReg = CondInstr->getOperand(3).getReg();
+  MachineInstr *CondZeroInstr = MRI.getVRegDef(CondZeroReg);
+  if (CondZeroInstr->getOpcode() != TargetOpcode::G_FCONSTANT ||
+      !CondZeroInstr->getOperand(1).getFPImm()->isZero())
+    return false;
+
+  // Check if select's false operand is the negation of the true operand.
+  Register TrueReg = MI.getOperand(2).getReg();
+  Register FalseReg = MI.getOperand(3).getReg();
+  MachineInstr *FalseInstr = MRI.getVRegDef(FalseReg);
+  if (FalseInstr->getOpcode() != TargetOpcode::G_FNEG)
+    return false;
+  if (TrueReg != FalseInstr->getOperand(1).getReg())
+    return false;
+
+  return true;
+}
+void applySPIRVFaceForward(MachineInstr &MI, MachineRegisterInfo &MRI,
+                           MachineIRBuilder &B) {
+
+  // Extract the operands for N, I, and Ng from the match criteria.
+  Register CondReg = MI.getOperand(1).getReg();
+  MachineInstr *CondInstr = MRI.getVRegDef(CondReg);
+  Register DotReg = CondInstr->getOperand(2).getReg();
+  MachineInstr *DotInstr = MRI.getVRegDef(DotReg);
+  Register DotOperand1, DotOperand2;
+  if (DotInstr->getOpcode() == TargetOpcode::G_FMUL) {
+    DotOperand1 = DotInstr->getOperand(1).getReg();
+    DotOperand2 = DotInstr->getOperand(2).getReg();
+  } else {
+    DotOperand1 = DotInstr->getOperand(2).getReg();
+    DotOperand2 = DotInstr->getOperand(3).getReg();
+  }
+  Register TrueReg = MI.getOperand(2).getReg();
+
+  // Remove the original `select` instruction.
+  Register ResultReg = MI.getOperand(0).getReg();
+  DebugLoc DL = MI.getDebugLoc();
+  MachineBasicBlock &MBB = *MI.getParent();
+  MachineBasicBlock::iterator InsertPt = MI.getIterator();
+
+  // Build the `spv_faceforward` intrinsic.
+  MachineInstrBuilder NewInstr =
+      BuildMI(MBB, InsertPt, DL, B.getTII().get(TargetOpcode::G_INTRINSIC));
+  NewInstr
+      .addDef(ResultReg)                          // Result register
+      .addIntrinsicID(Intrinsic::spv_faceforward) // Intrinsic ID
+      .addUse(TrueReg)                            // Operand N
+      .addUse(DotOperand1)                        // Operand I
+      .addUse(DotOperand2);                       // Operand Ng
+
+  SPIRVGlobalRegistry *GR =
+      MI.getMF()->getSubtarget<SPIRVSubtarget>().getSPIRVGlobalRegistry();
+  removeAllUses(CondReg, MRI, GR); // remove all uses of FCMP Result
+  GR->invalidateMachineInstr(CondInstr);
+  CondInstr->eraseFromParent(); // remove FCMP instruction
+}
+
 class SPIRVPreLegalizerCombinerImpl : public Combiner {
 protected:
   const CombinerHelper Helper;
diff --git a/llvm/test/CodeGen/SPIRV/GlobalISel/InstCombine/prelegalizercombiner-select-to-faceforward.mir b/llvm/test/CodeGen/SPIRV/GlobalISel/InstCombine/prelegalizercombiner-select-to-faceforward.mir
new file mode 100644
index 0000000000000..08f03942460ba
--- /dev/null
+++ b/llvm/test/CodeGen/SPIRV/GlobalISel/InstCombine/prelegalizercombiner-select-to-faceforward.mir
@@ -0,0 +1,63 @@
+# RUN: llc -verify-machineinstrs -O0 -mtriple spirv-unknown-unknown -run-pass=spirv-prelegalizer-combiner %s -o - | FileCheck %s
+# REQUIRES: asserts
+---
+name:            faceforward_instcombine_float
+tracksRegLiveness: true
+legalized: true
+body:             |
+  bb.1.entry:
+    ; CHECK-LABEL: name: faceforward_instcombine_float
+    ; CHECK-NOT: %9:_(s32) = G_FCONSTANT float 0.000000e+00
+    ; CHECK-NOT: %8:_(s32) = G_FMUL %1:fid, %2:fid
+    ; CHECK-NOT: %10:_(s1) = G_FCMP floatpred(olt), %8:_(s32), %9:_
+    ; CHECK-NOT: %11:_(s32) = G_FNEG %0:fid
+    ; CHECK-NOT: %12:id(s32) = G_SELECT %10:_(s1), %0:fid, %11:_
+    ; CHECK: %10:id(s32) = G_INTRINSIC intrinsic(@llvm.spv.faceforward), %2(s32), %3(s32), %4(s32)
+    %3:type(s64) = OpTypeFloat 32
+    %5:type(s64) = OpTypeFunction %3:type(s64), %3:type(s64), %3:type(s64), %3:type(s64)
+    OpName %0:fid(s32), 97
+    OpName %1:fid(s32), 98
+    OpName %2:fid(s32), 99
+    %4:iid(s64) = OpFunction %3:type(s64), 0, %5:type(s64)
+    %0:fid(s32) = OpFunctionParameter %3:type(s64)
+    %1:fid(s32) = OpFunctionParameter %3:type(s64)
+    %2:fid(s32) = OpFunctionParameter %3:type(s64)
+    OpName %4:iid(s64), 1701011814, 2003988326, 1600418401, 1953721961, 1651339107, 1600482921, 1634692198, 116
+    %9:_(s32) = G_FCONSTANT float 0.000000e+00
+    %8:_(s32) = G_FMUL %1:fid, %2:fid
+    %10:_(s1) = G_FCMP floatpred(olt), %8:_(s32), %9:_
+    %11:_(s32) = G_FNEG %0:fid
+    %12:id(s32) = G_SELECT %10:_(s1), %0:fid, %11:_
+    OpReturnValue %12:id(s32)
+---
+name:            faceforward_instcombine_float4
+tracksRegLiveness: true
+legalized: true
+body:             |
+  bb.1.entry:
+    ; CHECK-LABEL: name: faceforward_instcombine_float4
+    ; CHECK-NOT: %10:_(s32) = G_FCONSTANT float 0.000000e+00
+    ; CHECK-NOT: %9:_(s32) = G_INTRINSIC intrinsic(@llvm.spv.fdot), %1:vfid(<4 x s32>), %2:vfid(<4 x s32>)
+    ; CHECK-NOT: %11:_(s1) = G_FCMP floatpred(olt), %9:_(s32), %10:_
+    ; CHECK-NOT: %12:_(<4 x s32>) = G_FNEG %0:vfid
+    ; CHECK-NOT: %13:id(<4 x s32>) = G_SELECT %11:_(s1), %0:vfid, %12:_
+    ; CHECK: %11:id(<4 x s32>) = G_INTRINSIC intrinsic(@llvm.spv.faceforward), %3(<4 x s32>), %4(<4 x s32>), %5(<4 x s32>)
+    %4:type(s64) = OpTypeVector %3:type(s64), 4
+    %6:type(s64) = OpTypeFunction %4:type(s64), %4:type(s64), %4:type(s64), %4:type(s64)
+    %3:type(s64) = OpTypeFloat 32
+    OpName %0:vfid(<4 x s32>), 97
+    OpName %1:vfid(<4 x s32>), 98
+    OpName %2:vfid(<4 x s32>), 99
+    %5:iid(s64) = OpFunction %4:type(s64), 0, %6:type(s64)
+    %0:vfid(<4 x s32>) = OpFunctionParameter %4:type(s64)
+    %1:vfid(<4 x s32>) = OpFunctionParameter %4:type(s64)
+    %2:vfid(<4 x s32>) = OpFunctionParameter %4:type(s64)
+    OpName %5:iid(s64), 1701011814, 2003988326, 1600418401, 1953721961, 1651339107, 1600482921, 1634692198, 13428
+    OpDecorate %5:iid(s64), 41, 1701011814, 2003988326, 1600418401, 1953721961, 1651339107, 1600482921, 1634692198, 13428, 0
+    %10:_(s32) = G_FCONSTANT float 0.000000e+00
+    %9:_(s32) = G_INTRINSIC intrinsic(@llvm.spv.fdot), %1:vfid(<4 x s32>), %2:vfid(<4 x s32>)
+    %11:_(s1) = G_FCMP floatpred(olt), %9:_(s32), %10:_
+    %12:_(<4 x s32>) = G_FNEG %0:vfid
+    %13:id(<4 x s32>) = G_SELECT %11:_(s1), %0:vfid, %12:_
+    OpReturnValue %13:id(<4 x s32>)
+      
\ No newline at end of file
diff --git a/llvm/test/CodeGen/SPIRV/hlsl-intrinsics/faceforward.ll b/llvm/test/CodeGen/SPIRV/hlsl-intrinsics/faceforward.ll
index 22742169506f2..6811f95bbab17 100644
--- a/llvm/test/CodeGen/SPIRV/hlsl-intrinsics/faceforward.ll
+++ b/llvm/test/CodeGen/SPIRV/hlsl-intrinsics/faceforward.ll
@@ -1,7 +1,5 @@
-; RUN: llc -O0 -verify-machineinstrs -mtriple=spirv-unknown-unknown %s -o - | FileCheck %s
-; RUN: %if spirv-tools %{ llc -O0 -mtriple=spirv-unknown-unknown %s -o - -filetype=obj | spirv-val --target-env spv1.4 %}
-
-; FIXME(#136344): Change --target-env to vulkan1.3 and update this test accordingly once the issue is resolved.
+; RUN: llc -O0 -verify-machineinstrs -mtriple=spirv-vulkan1.3-unknown %s -o...
[truncated]

@llvmbot
Copy link
Member

llvmbot commented May 14, 2025

@llvm/pr-subscribers-hlsl

Author: Kaitlin Peng (kmpeng)

Changes

Tasks completed:

  • instCombine select(fcmp(dot(p2, p3), 0), p1, 0 - p1) to faceforward(p1, p2, p3)
  • Remove HLSL faceforward intrinsic's SPIR-V fast path
  • Add instCombine tests to prelegalizercombiner-select-to-faceforward.mir and faceforward.ll
  • Add CL extension error test llvm/test/CodeGen/SPIRV/opencl/faceforward-error.ll

Closes #137255.


Patch is 25.07 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/139959.diff

7 Files Affected:

  • (modified) clang/lib/Headers/hlsl/hlsl_intrinsic_helpers.h (-4)
  • (modified) clang/test/CodeGenHLSL/builtins/faceforward.hlsl (+40-16)
  • (modified) llvm/lib/Target/SPIRV/SPIRVCombine.td (+8-1)
  • (modified) llvm/lib/Target/SPIRV/SPIRVPreLegalizerCombiner.cpp (+100-11)
  • (added) llvm/test/CodeGen/SPIRV/GlobalISel/InstCombine/prelegalizercombiner-select-to-faceforward.mir (+63)
  • (modified) llvm/test/CodeGen/SPIRV/hlsl-intrinsics/faceforward.ll (+42-8)
  • (added) llvm/test/CodeGen/SPIRV/opencl/faceforward-error.ll (+13)
diff --git a/clang/lib/Headers/hlsl/hlsl_intrinsic_helpers.h b/clang/lib/Headers/hlsl/hlsl_intrinsic_helpers.h
index 4eb7b8f45c85a..e1996fd3e30ed 100644
--- a/clang/lib/Headers/hlsl/hlsl_intrinsic_helpers.h
+++ b/clang/lib/Headers/hlsl/hlsl_intrinsic_helpers.h
@@ -127,11 +127,7 @@ template <typename T> constexpr vector<T, 4> lit_impl(T NDotL, T NDotH, T M) {
 }
 
 template <typename T> constexpr T faceforward_impl(T N, T I, T Ng) {
-#if (__has_builtin(__builtin_spirv_faceforward))
-  return __builtin_spirv_faceforward(N, I, Ng);
-#else
   return select(dot(I, Ng) < 0, N, -N);
-#endif
 }
 
 template <typename T> constexpr T ldexp_impl(T X, T Exp) {
diff --git a/clang/test/CodeGenHLSL/builtins/faceforward.hlsl b/clang/test/CodeGenHLSL/builtins/faceforward.hlsl
index d2ece57aba4ae..f12f75f384964 100644
--- a/clang/test/CodeGenHLSL/builtins/faceforward.hlsl
+++ b/clang/test/CodeGenHLSL/builtins/faceforward.hlsl
@@ -12,8 +12,11 @@
 // CHECK: %hlsl.select.i = select reassoc nnan ninf nsz arcp afn i1 %cmp.i, half %{{.*}}, half %fneg.i
 // CHECK: ret half %hlsl.select.i
 // SPVCHECK-LABEL: test_faceforward_half
-// SPVCHECK: %spv.faceforward.i = call reassoc nnan ninf nsz arcp afn noundef half @llvm.spv.faceforward.f16(half %{{.*}}, half %{{.*}}, half %{{.*}})
-// SPVCHECK: ret half %spv.faceforward.i
+// SPVCHECK: %hlsl.dot.i = fmul reassoc nnan ninf nsz arcp afn half %{{.*}}, %{{.*}}
+// SPVCHECK: %cmp.i = fcmp reassoc nnan ninf nsz arcp afn olt half %hlsl.dot.i, 0xH0000
+// SPVCHECK: %fneg.i = fneg reassoc nnan ninf nsz arcp afn half %{{.*}}
+// SPVCHECK: %hlsl.select.i = select reassoc nnan ninf nsz arcp afn i1 %cmp.i, half %{{.*}}, half %fneg.i
+// SPVCHECK: ret half %hlsl.select.i
 half test_faceforward_half(half N, half I, half Ng) { return faceforward(N, I, Ng); }
 
 // CHECK-LABEL: test_faceforward_half2
@@ -23,8 +26,11 @@ half test_faceforward_half(half N, half I, half Ng) { return faceforward(N, I, N
 // CHECK: %hlsl.select.i = select reassoc nnan ninf nsz arcp afn i1 %cmp.i, <2 x half> %{{.*}}, <2 x half> %fneg.i
 // CHECK: ret <2 x half> %hlsl.select.i
 // SPVCHECK-LABEL: test_faceforward_half2
-// SPVCHECK: %spv.faceforward.i = call reassoc nnan ninf nsz arcp afn noundef <2 x half> @llvm.spv.faceforward.v2f16(<2 x half> %{{.*}}, <2 x half> %{{.*}}, <2 x half> %{{.*}})
-// SPVCHECK: ret <2 x half> %spv.faceforward.i
+// SPVCHECK: %hlsl.dot.i = call reassoc nnan ninf nsz arcp afn half @llvm.spv.fdot.v2f16(<2 x half> %{{.*}}, <2 x half> %{{.*}})
+// SPVCHECK: %cmp.i = fcmp reassoc nnan ninf nsz arcp afn olt half %hlsl.dot.i, 0xH0000
+// SPVCHECK: %fneg.i = fneg reassoc nnan ninf nsz arcp afn <2 x half> %{{.*}}
+// SPVCHECK: %hlsl.select.i = select reassoc nnan ninf nsz arcp afn i1 %cmp.i, <2 x half> %{{.*}}, <2 x half> %fneg.i
+// SPVCHECK: ret <2 x half> %hlsl.select.i
 half2 test_faceforward_half2(half2 N, half2 I, half2 Ng) { return faceforward(N, I, Ng); }
 
 // CHECK-LABEL: test_faceforward_half3
@@ -34,8 +40,11 @@ half2 test_faceforward_half2(half2 N, half2 I, half2 Ng) { return faceforward(N,
 // CHECK: %hlsl.select.i = select reassoc nnan ninf nsz arcp afn i1 %cmp.i, <3 x half> %{{.*}}, <3 x half> %fneg.i
 // CHECK: ret <3 x half> %hlsl.select.i
 // SPVCHECK-LABEL: test_faceforward_half3
-// SPVCHECK: %spv.faceforward.i = call reassoc nnan ninf nsz arcp afn noundef <3 x half> @llvm.spv.faceforward.v3f16(<3 x half> %{{.*}}, <3 x half> %{{.*}}, <3 x half> %{{.*}})
-// SPVCHECK: ret <3 x half> %spv.faceforward.i
+// SPVCHECK: %hlsl.dot.i = call reassoc nnan ninf nsz arcp afn half @llvm.spv.fdot.v3f16(<3 x half> %{{.*}}, <3 x half> %{{.*}})
+// SPVCHECK: %cmp.i = fcmp reassoc nnan ninf nsz arcp afn olt half %hlsl.dot.i, 0xH0000
+// SPVCHECK: %fneg.i = fneg reassoc nnan ninf nsz arcp afn <3 x half> %{{.*}}
+// SPVCHECK: %hlsl.select.i = select reassoc nnan ninf nsz arcp afn i1 %cmp.i, <3 x half> %{{.*}}, <3 x half> %fneg.i
+// SPVCHECK: ret <3 x half> %hlsl.select.i
 half3 test_faceforward_half3(half3 N, half3 I, half3 Ng) { return faceforward(N, I, Ng); }
 
 // CHECK-LABEL: test_faceforward_half4
@@ -45,8 +54,11 @@ half3 test_faceforward_half3(half3 N, half3 I, half3 Ng) { return faceforward(N,
 // CHECK: %hlsl.select.i = select reassoc nnan ninf nsz arcp afn i1 %cmp.i, <4 x half> %{{.*}}, <4 x half> %fneg.i
 // CHECK: ret <4 x half> %hlsl.select.i
 // SPVCHECK-LABEL: test_faceforward_half4
-// SPVCHECK: %spv.faceforward.i = call reassoc nnan ninf nsz arcp afn noundef <4 x half> @llvm.spv.faceforward.v4f16(<4 x half> %{{.*}}, <4 x half> %{{.*}}, <4 x half> %{{.*}})
-// SPVCHECK: ret <4 x half> %spv.faceforward.i
+// SPVCHECK: %hlsl.dot.i = call reassoc nnan ninf nsz arcp afn half @llvm.spv.fdot.v4f16(<4 x half> %{{.*}}, <4 x half> %{{.*}})
+// SPVCHECK: %cmp.i = fcmp reassoc nnan ninf nsz arcp afn olt half %hlsl.dot.i, 0xH0000
+// SPVCHECK: %fneg.i = fneg reassoc nnan ninf nsz arcp afn <4 x half> %{{.*}}
+// SPVCHECK: %hlsl.select.i = select reassoc nnan ninf nsz arcp afn i1 %cmp.i, <4 x half> %{{.*}}, <4 x half> %fneg.i
+// SPVCHECK: ret <4 x half> %hlsl.select.i
 half4 test_faceforward_half4(half4 N, half4 I, half4 Ng) { return faceforward(N, I, Ng); }
 
 // CHECK-LABEL: test_faceforward_float
@@ -56,8 +68,11 @@ half4 test_faceforward_half4(half4 N, half4 I, half4 Ng) { return faceforward(N,
 // CHECK: %hlsl.select.i = select reassoc nnan ninf nsz arcp afn i1 %cmp.i, float %{{.*}}, float %fneg.i
 // CHECK: ret float %hlsl.select.i
 // SPVCHECK-LABEL: test_faceforward_float
-// SPVCHECK: %spv.faceforward.i = call reassoc nnan ninf nsz arcp afn noundef float @llvm.spv.faceforward.f32(float %{{.*}}, float %{{.*}}, float %{{.*}})
-// SPVCHECK: ret float %spv.faceforward.i
+// SPVCHECK: %hlsl.dot.i = fmul reassoc nnan ninf nsz arcp afn float %{{.*}}, %{{.*}}
+// SPVCHECK: %cmp.i = fcmp reassoc nnan ninf nsz arcp afn olt float %hlsl.dot.i, 0.000000e+00
+// SPVCHECK: %fneg.i = fneg reassoc nnan ninf nsz arcp afn float %{{.*}}
+// SPVCHECK: %hlsl.select.i = select reassoc nnan ninf nsz arcp afn i1 %cmp.i, float %{{.*}}, float %fneg.i
+// SPVCHECK: ret float %hlsl.select.i
 float test_faceforward_float(float N, float I, float Ng) { return faceforward(N, I, Ng); }
 
 // CHECK-LABEL: test_faceforward_float2
@@ -67,8 +82,11 @@ float test_faceforward_float(float N, float I, float Ng) { return faceforward(N,
 // CHECK: %hlsl.select.i = select reassoc nnan ninf nsz arcp afn i1 %cmp.i, <2 x float> %{{.*}}, <2 x float> %fneg.i
 // CHECK: ret <2 x float> %hlsl.select.i
 // SPVCHECK-LABEL: test_faceforward_float2
-// SPVCHECK: %spv.faceforward.i = call reassoc nnan ninf nsz arcp afn noundef <2 x float> @llvm.spv.faceforward.v2f32(<2 x float> %{{.*}}, <2 x float> %{{.*}}, <2 x float> %{{.*}})
-// SPVCHECK: ret <2 x float> %spv.faceforward.i
+// SPVCHECK: %hlsl.dot.i = call reassoc nnan ninf nsz arcp afn float @llvm.spv.fdot.v2f32(<2 x float> %{{.*}}, <2 x float> %{{.*}})
+// SPVCHECK: %cmp.i = fcmp reassoc nnan ninf nsz arcp afn olt float %hlsl.dot.i, 0.000000e+00
+// SPVCHECK: %fneg.i = fneg reassoc nnan ninf nsz arcp afn <2 x float> %{{.*}}
+// SPVCHECK: %hlsl.select.i = select reassoc nnan ninf nsz arcp afn i1 %cmp.i, <2 x float> %{{.*}}, <2 x float> %fneg.i
+// SPVCHECK: ret <2 x float> %hlsl.select.i
 float2 test_faceforward_float2(float2 N, float2 I, float2 Ng) { return faceforward(N, I, Ng); }
 
 // CHECK-LABEL: test_faceforward_float3
@@ -78,8 +96,11 @@ float2 test_faceforward_float2(float2 N, float2 I, float2 Ng) { return faceforwa
 // CHECK: %hlsl.select.i = select reassoc nnan ninf nsz arcp afn i1 %cmp.i, <3 x float> %{{.*}}, <3 x float> %fneg.i
 // CHECK: ret <3 x float> %hlsl.select.i
 // SPVCHECK-LABEL: test_faceforward_float3
-// SPVCHECK: %spv.faceforward.i = call reassoc nnan ninf nsz arcp afn noundef <3 x float> @llvm.spv.faceforward.v3f32(<3 x float> %{{.*}}, <3 x float> %{{.*}}, <3 x float> %{{.*}})
-// SPVCHECK: ret <3 x float> %spv.faceforward.i
+// SPVCHECK: %hlsl.dot.i = call reassoc nnan ninf nsz arcp afn float @llvm.spv.fdot.v3f32(<3 x float> %{{.*}}, <3 x float> %{{.*}})
+// SPVCHECK: %cmp.i = fcmp reassoc nnan ninf nsz arcp afn olt float %hlsl.dot.i, 0.000000e+00
+// SPVCHECK: %fneg.i = fneg reassoc nnan ninf nsz arcp afn <3 x float> %{{.*}}
+// SPVCHECK: %hlsl.select.i = select reassoc nnan ninf nsz arcp afn i1 %cmp.i, <3 x float> %{{.*}}, <3 x float> %fneg.i
+// SPVCHECK: ret <3 x float> %hlsl.select.i
 float3 test_faceforward_float3(float3 N, float3 I, float3 Ng) { return faceforward(N, I, Ng); }
 
 // CHECK-LABEL: test_faceforward_float4
@@ -89,6 +110,9 @@ float3 test_faceforward_float3(float3 N, float3 I, float3 Ng) { return faceforwa
 // CHECK: %hlsl.select.i = select reassoc nnan ninf nsz arcp afn i1 %cmp.i, <4 x float> %{{.*}}, <4 x float> %fneg.i
 // CHECK: ret <4 x float> %hlsl.select.i
 // SPVCHECK-LABEL: test_faceforward_float4
-// SPVCHECK: %spv.faceforward.i = call reassoc nnan ninf nsz arcp afn noundef <4 x float> @llvm.spv.faceforward.v4f32(<4 x float> %{{.*}}, <4 x float> %{{.*}}, <4 x float> %{{.*}})
-// SPVCHECK: ret <4 x float> %spv.faceforward.i
+// SPVCHECK: %hlsl.dot.i = call reassoc nnan ninf nsz arcp afn float @llvm.spv.fdot.v4f32(<4 x float> %{{.*}}, <4 x float> %{{.*}})
+// SPVCHECK: %cmp.i = fcmp reassoc nnan ninf nsz arcp afn olt float %hlsl.dot.i, 0.000000e+00
+// SPVCHECK: %fneg.i = fneg reassoc nnan ninf nsz arcp afn <4 x float> %{{.*}}
+// SPVCHECK: %hlsl.select.i = select reassoc nnan ninf nsz arcp afn i1 %cmp.i, <4 x float> %{{.*}}, <4 x float> %fneg.i
+// SPVCHECK: ret <4 x float> %hlsl.select.i
 float4 test_faceforward_float4(float4 N, float4 I, float4 Ng) { return faceforward(N, I, Ng); }
diff --git a/llvm/lib/Target/SPIRV/SPIRVCombine.td b/llvm/lib/Target/SPIRV/SPIRVCombine.td
index 6f726e024de52..78a57146e61d2 100644
--- a/llvm/lib/Target/SPIRV/SPIRVCombine.td
+++ b/llvm/lib/Target/SPIRV/SPIRVCombine.td
@@ -15,8 +15,15 @@ def vector_length_sub_to_distance_lowering : GICombineRule <
   (apply [{ applySPIRVDistance(*${root}, MRI, B); }])
 >;
 
+def vector_select_to_faceforward_lowering : GICombineRule <
+  (defs root:$root),
+  (match (wip_match_opcode G_SELECT):$root,
+          [{ return matchSelectToFaceForward(*${root}, MRI); }]),
+  (apply [{ applySPIRVFaceForward(*${root}, MRI, B); }])
+>;
+
 def SPIRVPreLegalizerCombiner
     : GICombiner<"SPIRVPreLegalizerCombinerImpl",
-                       [vector_length_sub_to_distance_lowering]> {
+                       [vector_length_sub_to_distance_lowering, vector_select_to_faceforward_lowering]> {
     let CombineAllMethodName = "tryCombineAllImpl";
 }
diff --git a/llvm/lib/Target/SPIRV/SPIRVPreLegalizerCombiner.cpp b/llvm/lib/Target/SPIRV/SPIRVPreLegalizerCombiner.cpp
index c96ee6b02491a..82c5cbf75b849 100644
--- a/llvm/lib/Target/SPIRV/SPIRVPreLegalizerCombiner.cpp
+++ b/llvm/lib/Target/SPIRV/SPIRVPreLegalizerCombiner.cpp
@@ -50,6 +50,18 @@ namespace {
 #include "SPIRVGenPreLegalizeGICombiner.inc"
 #undef GET_GICOMBINER_TYPES
 
+static void removeAllUses(Register Reg, MachineRegisterInfo &MRI,
+                          SPIRVGlobalRegistry *GR) {
+  SmallVector<MachineInstr *, 4> UsesToErase(
+      llvm::make_pointer_range(MRI.use_instructions(Reg)));
+
+  // calling eraseFromParent too early invalidates the iterator.
+  for (auto *MIToErase : UsesToErase) {
+    GR->invalidateMachineInstr(MIToErase);
+    MIToErase->eraseFromParent();
+  }
+}
+
 /// This match is part of a combine that
 /// rewrites length(X - Y) to distance(X, Y)
 ///   (f32 (g_intrinsic length
@@ -98,21 +110,98 @@ void applySPIRVDistance(MachineInstr &MI, MachineRegisterInfo &MRI,
 
   SPIRVGlobalRegistry *GR =
       MI.getMF()->getSubtarget<SPIRVSubtarget>().getSPIRVGlobalRegistry();
-  auto RemoveAllUses = [&](Register Reg) {
-    SmallVector<MachineInstr *, 4> UsesToErase(
-        llvm::make_pointer_range(MRI.use_instructions(Reg)));
-
-    // calling eraseFromParent to early invalidates the iterator.
-    for (auto *MIToErase : UsesToErase) {
-      GR->invalidateMachineInstr(MIToErase);
-      MIToErase->eraseFromParent();
-    }
-  };
-  RemoveAllUses(SubDestReg);   // remove all uses of FSUB Result
+  removeAllUses(SubDestReg, MRI, GR); // remove all uses of FSUB Result
   GR->invalidateMachineInstr(SubInstr);
   SubInstr->eraseFromParent(); // remove FSUB instruction
 }
 
+/// This match is part of a combine that
+/// rewrites select(fcmp(dot(I, Ng), 0), N, 0 - N) to faceforward(N, I, Ng)
+///   (vXf32 (g_select
+///             (g_fcmp
+///                (g_intrinsic dot(vXf32 I) (vXf32 Ng)
+///                 0)
+///             (vXf32 N)
+///             (vXf32 g_fsub (0) (vXf32 N))))
+/// ->
+///   (vXf32 (g_intrinsic faceforward
+///             (vXf32 N) (vXf32 I) (vXf32 Ng)))
+///
+bool matchSelectToFaceForward(MachineInstr &MI, MachineRegisterInfo &MRI) {
+  if (MI.getOpcode() != TargetOpcode::G_SELECT)
+    return false;
+
+  // Check if select's condition is a comparison between a dot product and 0.
+  Register CondReg = MI.getOperand(1).getReg();
+  MachineInstr *CondInstr = MRI.getVRegDef(CondReg);
+  if (!CondInstr || CondInstr->getOpcode() != TargetOpcode::G_FCMP)
+    return false;
+
+  Register DotReg = CondInstr->getOperand(2).getReg();
+  MachineInstr *DotInstr = MRI.getVRegDef(DotReg);
+  if (DotInstr->getOpcode() != TargetOpcode::G_FMUL &&
+      (DotInstr->getOpcode() != TargetOpcode::G_INTRINSIC ||
+       cast<GIntrinsic>(DotInstr)->getIntrinsicID() != Intrinsic::spv_fdot))
+    return false;
+
+  Register CondZeroReg = CondInstr->getOperand(3).getReg();
+  MachineInstr *CondZeroInstr = MRI.getVRegDef(CondZeroReg);
+  if (CondZeroInstr->getOpcode() != TargetOpcode::G_FCONSTANT ||
+      !CondZeroInstr->getOperand(1).getFPImm()->isZero())
+    return false;
+
+  // Check if select's false operand is the negation of the true operand.
+  Register TrueReg = MI.getOperand(2).getReg();
+  Register FalseReg = MI.getOperand(3).getReg();
+  MachineInstr *FalseInstr = MRI.getVRegDef(FalseReg);
+  if (FalseInstr->getOpcode() != TargetOpcode::G_FNEG)
+    return false;
+  if (TrueReg != FalseInstr->getOperand(1).getReg())
+    return false;
+
+  return true;
+}
+void applySPIRVFaceForward(MachineInstr &MI, MachineRegisterInfo &MRI,
+                           MachineIRBuilder &B) {
+
+  // Extract the operands for N, I, and Ng from the match criteria.
+  Register CondReg = MI.getOperand(1).getReg();
+  MachineInstr *CondInstr = MRI.getVRegDef(CondReg);
+  Register DotReg = CondInstr->getOperand(2).getReg();
+  MachineInstr *DotInstr = MRI.getVRegDef(DotReg);
+  Register DotOperand1, DotOperand2;
+  if (DotInstr->getOpcode() == TargetOpcode::G_FMUL) {
+    DotOperand1 = DotInstr->getOperand(1).getReg();
+    DotOperand2 = DotInstr->getOperand(2).getReg();
+  } else {
+    DotOperand1 = DotInstr->getOperand(2).getReg();
+    DotOperand2 = DotInstr->getOperand(3).getReg();
+  }
+  Register TrueReg = MI.getOperand(2).getReg();
+
+  // Remove the original `select` instruction.
+  Register ResultReg = MI.getOperand(0).getReg();
+  DebugLoc DL = MI.getDebugLoc();
+  MachineBasicBlock &MBB = *MI.getParent();
+  MachineBasicBlock::iterator InsertPt = MI.getIterator();
+
+  // Build the `spv_faceforward` intrinsic.
+  MachineInstrBuilder NewInstr =
+      BuildMI(MBB, InsertPt, DL, B.getTII().get(TargetOpcode::G_INTRINSIC));
+  NewInstr
+      .addDef(ResultReg)                          // Result register
+      .addIntrinsicID(Intrinsic::spv_faceforward) // Intrinsic ID
+      .addUse(TrueReg)                            // Operand N
+      .addUse(DotOperand1)                        // Operand I
+      .addUse(DotOperand2);                       // Operand Ng
+
+  SPIRVGlobalRegistry *GR =
+      MI.getMF()->getSubtarget<SPIRVSubtarget>().getSPIRVGlobalRegistry();
+  removeAllUses(CondReg, MRI, GR); // remove all uses of FCMP Result
+  GR->invalidateMachineInstr(CondInstr);
+  CondInstr->eraseFromParent(); // remove FCMP instruction
+}
+
 class SPIRVPreLegalizerCombinerImpl : public Combiner {
 protected:
   const CombinerHelper Helper;
diff --git a/llvm/test/CodeGen/SPIRV/GlobalISel/InstCombine/prelegalizercombiner-select-to-faceforward.mir b/llvm/test/CodeGen/SPIRV/GlobalISel/InstCombine/prelegalizercombiner-select-to-faceforward.mir
new file mode 100644
index 0000000000000..08f03942460ba
--- /dev/null
+++ b/llvm/test/CodeGen/SPIRV/GlobalISel/InstCombine/prelegalizercombiner-select-to-faceforward.mir
@@ -0,0 +1,63 @@
+# RUN: llc -verify-machineinstrs -O0 -mtriple spirv-unknown-unknown -run-pass=spirv-prelegalizer-combiner %s -o - | FileCheck %s
+# REQUIRES: asserts
+---
+name:            faceforward_instcombine_float
+tracksRegLiveness: true
+legalized: true
+body:             |
+  bb.1.entry:
+    ; CHECK-LABEL: name: faceforward_instcombine_float
+    ; CHECK-NOT: %9:_(s32) = G_FCONSTANT float 0.000000e+00
+    ; CHECK-NOT: %8:_(s32) = G_FMUL %1:fid, %2:fid
+    ; CHECK-NOT: %10:_(s1) = G_FCMP floatpred(olt), %8:_(s32), %9:_
+    ; CHECK-NOT: %11:_(s32) = G_FNEG %0:fid
+    ; CHECK-NOT: %12:id(s32) = G_SELECT %10:_(s1), %0:fid, %11:_
+    ; CHECK: %10:id(s32) = G_INTRINSIC intrinsic(@llvm.spv.faceforward), %2(s32), %3(s32), %4(s32)
+    %3:type(s64) = OpTypeFloat 32
+    %5:type(s64) = OpTypeFunction %3:type(s64), %3:type(s64), %3:type(s64), %3:type(s64)
+    OpName %0:fid(s32), 97
+    OpName %1:fid(s32), 98
+    OpName %2:fid(s32), 99
+    %4:iid(s64) = OpFunction %3:type(s64), 0, %5:type(s64)
+    %0:fid(s32) = OpFunctionParameter %3:type(s64)
+    %1:fid(s32) = OpFunctionParameter %3:type(s64)
+    %2:fid(s32) = OpFunctionParameter %3:type(s64)
+    OpName %4:iid(s64), 1701011814, 2003988326, 1600418401, 1953721961, 1651339107, 1600482921, 1634692198, 116
+    %9:_(s32) = G_FCONSTANT float 0.000000e+00
+    %8:_(s32) = G_FMUL %1:fid, %2:fid
+    %10:_(s1) = G_FCMP floatpred(olt), %8:_(s32), %9:_
+    %11:_(s32) = G_FNEG %0:fid
+    %12:id(s32) = G_SELECT %10:_(s1), %0:fid, %11:_
+    OpReturnValue %12:id(s32)
+---
+name:            faceforward_instcombine_float4
+tracksRegLiveness: true
+legalized: true
+body:             |
+  bb.1.entry:
+    ; CHECK-LABEL: name: faceforward_instcombine_float4
+    ; CHECK-NOT: %10:_(s32) = G_FCONSTANT float 0.000000e+00
+    ; CHECK-NOT: %9:_(s32) = G_INTRINSIC intrinsic(@llvm.spv.fdot), %1:vfid(<4 x s32>), %2:vfid(<4 x s32>)
+    ; CHECK-NOT: %11:_(s1) = G_FCMP floatpred(olt), %9:_(s32), %10:_
+    ; CHECK-NOT: %12:_(<4 x s32>) = G_FNEG %0:vfid
+    ; CHECK-NOT: %13:id(<4 x s32>) = G_SELECT %11:_(s1), %0:vfid, %12:_
+    ; CHECK: %11:id(<4 x s32>) = G_INTRINSIC intrinsic(@llvm.spv.faceforward), %3(<4 x s32>), %4(<4 x s32>), %5(<4 x s32>)
+    %4:type(s64) = OpTypeVector %3:type(s64), 4
+    %6:type(s64) = OpTypeFunction %4:type(s64), %4:type(s64), %4:type(s64), %4:type(s64)
+    %3:type(s64) = OpTypeFloat 32
+    OpName %0:vfid(<4 x s32>), 97
+    OpName %1:vfid(<4 x s32>), 98
+    OpName %2:vfid(<4 x s32>), 99
+    %5:iid(s64) = OpFunction %4:type(s64), 0, %6:type(s64)
+    %0:vfid(<4 x s32>) = OpFunctionParameter %4:type(s64)
+    %1:vfid(<4 x s32>) = OpFunctionParameter %4:type(s64)
+    %2:vfid(<4 x s32>) = OpFunctionParameter %4:type(s64)
+    OpName %5:iid(s64), 1701011814, 2003988326, 1600418401, 1953721961, 1651339107, 1600482921, 1634692198, 13428
+    OpDecorate %5:iid(s64), 41, 1701011814, 2003988326, 1600418401, 1953721961, 1651339107, 1600482921, 1634692198, 13428, 0
+    %10:_(s32) = G_FCONSTANT float 0.000000e+00
+    %9:_(s32) = G_INTRINSIC intrinsic(@llvm.spv.fdot), %1:vfid(<4 x s32>), %2:vfid(<4 x s32>)
+    %11:_(s1) = G_FCMP floatpred(olt), %9:_(s32), %10:_
+    %12:_(<4 x s32>) = G_FNEG %0:vfid
+    %13:id(<4 x s32>) = G_SELECT %11:_(s1), %0:vfid, %12:_
+    OpReturnValue %13:id(<4 x s32>)
+      
\ No newline at end of file
diff --git a/llvm/test/CodeGen/SPIRV/hlsl-intrinsics/faceforward.ll b/llvm/test/CodeGen/SPIRV/hlsl-intrinsics/faceforward.ll
index 22742169506f2..6811f95bbab17 100644
--- a/llvm/test/CodeGen/SPIRV/hlsl-intrinsics/faceforward.ll
+++ b/llvm/test/CodeGen/SPIRV/hlsl-intrinsics/faceforward.ll
@@ -1,7 +1,5 @@
-; RUN: llc -O0 -verify-machineinstrs -mtriple=spirv-unknown-unknown %s -o - | FileCheck %s
-; RUN: %if spirv-tools %{ llc -O0 -mtriple=spirv-unknown-unknown %s -o - -filetype=obj | spirv-val --target-env spv1.4 %}
-
-; FIXME(#136344): Change --target-env to vulkan1.3 and update this test accordingly once the issue is resolved.
+; RUN: llc -O0 -verify-machineinstrs -mtriple=spirv-vulkan1.3-unknown %s -o...
[truncated]

Copy link
Contributor

@arsenm arsenm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Description mentions instcombine but this has nothing to do with instcombine

Copy link
Contributor

@arsenm arsenm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Description should not mention instcombine, instcombine is not related

Copy link
Contributor

@s-perron s-perron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a minor concern about how specific the N and 0-N checks are. Otherwise this looks good to me.

@kmpeng kmpeng changed the title [SPIRV] Add PreLegalizer instCombine for faceforward [SPIRV] Add PreLegalizer pattern matching for faceforward May 16, 2025
@kmpeng kmpeng force-pushed the faceforward-patternmatch branch from 5cb68c5 to 6d19a11 Compare September 12, 2025 21:44
@kmpeng kmpeng force-pushed the faceforward-patternmatch branch 2 times, most recently from 67cad12 to 6868f35 Compare September 24, 2025 21:16
Comment on lines 150 to 151
if (DotInstr->getOpcode() != TargetOpcode::G_INTRINSIC ||
cast<GIntrinsic>(DotInstr)->getIntrinsicID() != Intrinsic::spv_fdot) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we don't have a matcher for intrinsic that takes the Intrinsic ID as an argument, there should be one

Copy link
Contributor Author

@kmpeng kmpeng Oct 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still not very familiar with mi_match so I'm not exactly sure how I would write this. Also, should this be done in this patch?

@kmpeng kmpeng force-pushed the faceforward-patternmatch branch from 64df069 to 900b7bb Compare October 13, 2025 18:44
@kmpeng kmpeng requested a review from arsenm October 14, 2025 18:02
CmpInst::Predicate Pred;
if (!mi_match(CondReg, MRI,
m_GFCmp(m_Pred(Pred), m_Reg(DotReg), m_Reg(CondZeroReg))) ||
Pred != CmpInst::FCMP_OLT)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be overly specialized to ordered and less than? What about these 16 other fcmp condition codes? https://llvm.org/docs/LangRef.html#fcmp-instruction

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my concern is what if an optimization pass comes along and rewrites our faceforward implementation and the condition changes for the comparison. How robust are we to handling something like this?

Comment on lines +150 to +153
if (!mi_match(DotReg, MRI, m_GFMul(m_Reg(DotOperand1), m_Reg(DotOperand2)))) {
DotOperand1 = DotInstr->getOperand(2).getReg();
DotOperand2 = DotInstr->getOperand(3).getReg();
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we do mi_match both in matchSelectToFaceForward and in applySPIRVFaceForward? I figured apply would be just do the transformation. The only matching i'm not seeing in apply is the dot product intrinsic matching.

Comment on lines +127 to +128
if (TrueInstr->getOpcode() == TargetOpcode::G_BUILD_VECTOR &&
FalseInstr->getOpcode() == TargetOpcode::G_BUILD_VECTOR &&
Copy link
Member

@farzonl farzonl Oct 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the negated-constant handling too narrow? The “opposites” check handles scalars and G_BUILD_VECTOR of literals, but it isn'y clear if this would catch common splat forms (e.g., splat from G_FNEG of a scalar, insert/extract builds, or fmul-by--1.0). Will all the splat of scalar to vector be converted to vectors by the time we get to the SPIRV backend?

/// (vXf32 (g_intrinsic faceforward
/// (vXf32 N) (vXf32 I) (vXf32 Ng)))
///
/// This only works for Vulkan targets.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// This only works for Vulkan targets.
/// This only works for Vulkan shader targets.

}

const ConstantFP *ZeroVal;
if (!mi_match(CondZeroReg, MRI, m_GFCst(ZeroVal)) || !ZeroVal->isZero())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think APFloats can be -0.0 and 0.0. Would be good to make sure in a test case that both work to get past this condition.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backend:SPIR-V backend:X86 clang:headers Headers provided by Clang, e.g. for intrinsics clang Clang issues not falling into any other category HLSL HLSL Language Support llvm:globalisel

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[SPIRV] Add PreLegalizer pattern matching for faceforward GL extension

5 participants